Show a more specific error message if config file is not writable #11663

burned42 · 2018-10-07T13:27:08Z

When I tried reproducing the problem described in #8580 I noticed that there is only a very generic error message displayed (like in the attached screenshot in this issue) when the config file does not exist and the config directory is not writable, so e.g. if you set up a new installation and didn't adjust the permissions correctly.

So this PR should introduce a more helpful error message for that case.

Signed-off-by: Bernd Stellwag <burned@zerties.org>

burned42 · 2018-10-07T14:27:23Z

In the README.md it says I should mention the people from the according issue, so I guess that's for having the right people notified for reviewing the code, right? So I'll just mention the ones marked as 'Member' I guess: @MorrisJobke @rullzer

Apparently the CI is currently failing but maybe someone of you could also give me a hint on how to debug those failing CI checks. Until now I didn't find out how to run the tests manually to get those errors. When running the tests locally they are all passing, so I don't really know what the issue is with the failing checks.

kesselb

Thank you for your contribution 👍

lib/base.php

kesselb · 2018-10-07T18:11:48Z

lib/private/Config.php

-		touch ($this->configFilePath);
+		// Check if config directory/file is writable and/or create the config file
+		if (
+			(!is_file($this->configFilePath) && !is_writable(dirname($this->configFilePath)))


Would !is_file($this->configFilePath) && !touch($this->configFilePath) work? I guess when either directory or file is not writable touch will fail anyway?

I think so 👍

Technically you are correct, touch will fail, but when it fails it fails with a PHP Notice generated. I tried to prevent this by first making sure the touch wouldn't fail due to missing write permissions. Another problem is that touch can succeed if the file exists and the current user is the owner of the file but has no write permissions, check https://stackoverflow.com/a/990893 for more information about this.

On further testing I also noticed that we should first touch and then check for is_writable because for a non existing file is_writable also returns false, see 5b2cf3c

I just found this. Just wondering is these function broken? I guess when config not writable there should be an error.

server/lib/base.php

Lines 233 to 268 in 40d185e

public static function checkConfig() {

$l = \OC::$server->getL10N('lib');

// Create config if it does not already exist

$configFilePath = self::$configDir .'/config.php';

if(!file_exists($configFilePath)) {

@touch($configFilePath);

}

// Check if config is writable

$configFileWritable = is_writable($configFilePath);

if (!$configFileWritable && !OC_Helper::isReadOnlyConfigEnabled()

|| !$configFileWritable && \OCP\Util::needUpgrade()) {

$urlGenerator = \OC::$server->getURLGenerator();

if (self::$CLI) {

echo $l->t('Cannot write into "config" directory!')."\n";

echo $l->t('This can usually be fixed by giving the webserver write access to the config directory')."\n";

echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-dir_permissions') ])."\n";

echo "\n";

echo $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.')."\n";

echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-config') ])."\n";

exit;

} else {

OC_Template::printErrorPage(

$l->t('Cannot write into "config" directory!'),

$l->t('This can usually be fixed by giving the webserver write access to the config directory. See %s',

[ $urlGenerator->linkToDocs('admin-dir_permissions') ]) . '. '

. $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it. See %s',

[ $urlGenerator->linkToDocs('admin-config') ] ),

503

);

}

}

}

PhpStorm tells me that !$configFileWritable && !OC_Helper::isReadOnlyConfigEnabled() || !$configFileWritable && \OCP\Util::needUpgrade() may not work as expected.

There seems to be a config parameter config_is_read_only which can be set to true if the config file is intentionally not writable, so I guess there shouldn't always be an error if the config file is not writable.

I guess PhpStorm is reporting this because there are no parentheses to clarify the intended order/grouping of those statements, but it seems to do the right thing. At least it makes sense to me, it goes into the if (displaying an error) if either the config file is not writable and not configured to be readonly or if the config file is not writable but an upgrade should be done (which would need to alter the config file).

Does it makes sense to add a check "is directory writable" to the function above? What do you think is the current check to late? I guess your check is executed much earlier.

I'm not sure if I understand, where exactly would you suggest adding a check 'is directory writable' and how would that help?

I'm not sure if checkConfig is called too late, 'my check' is called a bit earlier in base.php via self::initSession() which might try to create a config.php to set an instance ID. But I don't know if changing the order would be better or not. I guess it wouldn't really matter, we just would have to change checkConfig a bit.
checkConfig is using OC_Template::printErrorPage which as I already found out in #11663 (comment) tries to do some DB queries which of course can fail if there is no config.php with some DB settings.
But as 'my check' is running before that it shouldn't happen because otherwise it wouldn't even get that far to call checkConfig.

kesselb · 2018-10-07T18:15:18Z

You can check ci here https://drone.nextcloud.com/nextcloud/server/11179. I'm not sure if chmod works on drone.

burned42 · 2018-10-08T17:00:18Z

You can check ci here https://drone.nextcloud.com/nextcloud/server/11179. I'm not sure if chmod works on drone.

@danielkesselberg Thanks for the hint. What I don't understand is that for only 4 out of that big list of checks this one unit test fails, but I don't really know drone or the setup so maybe that is totally reasonable.

So if chmod is not working on drone (or some special configuration that is used in those 4 failing test?) that would explain why this test isn't working. I am not really familiar with drone, I found some documentation but nothing about chmod not working or similar. Is there some way to debug this? Or should I just use some commits with a bit of debugging output to check this theory?

burned42 · 2018-10-08T20:11:09Z

You can check ci here https://drone.nextcloud.com/nextcloud/server/11179. I'm not sure if chmod works on drone.

@danielkesselberg Thanks for the hint. What I don't understand is that for only 4 out of that big list of checks this one unit test fails, but I don't really know drone or the setup so maybe that is totally reasonable.

So if chmod is not working on drone (or some special configuration that is used in those 4 failing test?) that would explain why this test isn't working. I am not really familiar with drone, I found some documentation but nothing about chmod not working or similar. Is there some way to debug this? Or should I just use some commits with a bit of debugging output to check this theory?

@danielkesselberg Alright, I added some checks in b5616fd to make sure the file/directory permissions got applied correctly. And as it seems your assumption that chmod isn't working as expected on drone seems to be correct. So, should I just leave this commit in, or is there another solution to this problem?

I also found out that almost all other tests which include a chmod are also including a $this->markTestSkipped(), so this really seems to be a common problem.

OC_Util::getInstanceId() does try to set a new value if currently there is no instance id set. This could lead to an exception if the config file is not writable. Signed-off-by: Bernd Stellwag <burned@zerties.org>

Signed-off-by: Bernd Stellwag <burned@zerties.org>

burned42 · 2018-10-15T15:43:42Z

Is there anything that I should/could do currently? Or should I just wait for your review? :)

MorrisJobke · 2018-10-17T10:25:01Z

Is there anything that I should/could do currently? Or should I just wait for your review? :)

Basically somebody needs time to look into this. I'm currently quite full with other more urgent tasks. Sorry :/

MorrisJobke · 2018-11-05T23:28:35Z

@rullzer @blizzz @nickvergessen Mind to look into this one?

ChristophWurst · 2019-10-24T12:58:02Z

@burned42 could you update this branch to the latest master please? Let's see if the CI issues magically disappeared. Otherwise we have to have another look

Code looks fine otherwise 👍

burned42 · 2019-10-24T16:38:53Z

@ChristophWurst honestly, I already kind of forgot about this PR :D but I just pulled latest master and merged it into this branch (no merge conflicts fortunately). So let's see what the CI says.

ChristophWurst · 2019-10-24T17:14:11Z

@ChristophWurst honestly, I already kind of forgot about this PR :D but I just pulled latest master and merged it into this branch (no merge conflicts fortunately). So let's see what the CI says.

No worries :) Thanks for the quick action.

Looks like CI is happy (two unrelated failures can be ignored) 🎉 🎆

kesselb · 2019-10-24T20:40:34Z

I'm not a fan. Additional complexity without fixing anything.

We run into some issues while initializing the sessions. Actually there is no instance id yet. I think we should fix this issue by generating the instance id during the setup.
There is checkConfig method with the same check. It also creates the config.php on demand. Wouldn't it be easier to call checkConfig before initSession? (Probably something like fail if readonly config but no instance id should be added).

server/lib/base.php

Lines 639 to 645 in de69403

    
           \OC::$server->getEventLogger()->start('init_session', 'Initialize session'); 
        
           OC_App::loadApps(array('session')); 
        
           if (!self::$CLI) { 
        
           	self::initSession(); 
        
           } 
        
           \OC::$server->getEventLogger()->end('init_session'); 
        
           self::checkConfig();

kesselb · 2019-10-25T19:00:18Z

I'm not a fan. Additional complexity without fixing anything.

That probably sounded a bit rude. I still think it's more a theoretical issue because the setup needs a writable config directory anyway and it's not clear to me how you can end up in the above situation but fine by me ;)

nickvergessen · 2019-12-03T18:24:14Z

lib/base.php

+
+			http_response_code(503);
+			OC_Util::addStyle('guest');
+			OC_Template::printGuestPage('', 'error', ['errors' => [$error]]);


While I understand the intention of this PR, trying to print a page when config has an error is a bad idea. This will try to load SCSS stuff which might also fail to be written, etc, and then you end up with a blank page instead of message which at least gives you a hint what is wrong.

burned42 · 2019-12-03T21:00:12Z

Alright, I understand that, no problem. I just found the issue during last years hacktoberfest and tried to solve it. So as it seems like my solution isn't really sufficient to solve the problem I guess I'll just close the PR and maybe someone else (who maybe is also a bit more familiar with the codebase) will come up with a different solution to the problem ;)

ChristophWurst · 2019-12-18T14:48:35Z

Thanks for your effort nevertheless!

Check if the config file is writable and throw an exception if it's not

e44ff03

Signed-off-by: Bernd Stellwag <burned@zerties.org>

burned42 changed the title ~~Show a more specific error message if config file is not writable~~ [WIP] Show a more specific error message if config file is not writable Oct 7, 2018

burned42 force-pushed the show_error_if_config_not_writable branch from 5c84b2f to e37e248 Compare October 7, 2018 13:37

kesselb reviewed Oct 7, 2018

View reviewed changes

kesselb added this to the Nextcloud 15 milestone Oct 7, 2018

kesselb requested review from MorrisJobke and rullzer October 7, 2018 18:13

kesselb added 3. to review Waiting for reviews enhancement labels Oct 7, 2018

burned42 force-pushed the show_error_if_config_not_writable branch 2 times, most recently from 1e8d596 to b5616fd Compare October 8, 2018 19:57

burned42 force-pushed the show_error_if_config_not_writable branch from b5616fd to 83086b3 Compare October 10, 2018 16:19

burned42 changed the title ~~[WIP] Show a more specific error message if config file is not writable~~ Show a more specific error message if config file is not writable Oct 10, 2018

burned42 added 4 commits October 10, 2018 19:59

If there are exceptions thrown, display an error page

7b7d9fe

OC_Util::getInstanceId() does try to set a new value if currently there is no instance id set. This could lead to an exception if the config file is not writable. Signed-off-by: Bernd Stellwag <burned@zerties.org>

It's also ok if config dir is not writable but config file itself is

ac1d3e6

Signed-off-by: Bernd Stellwag <burned@zerties.org>

Make sure creating a new config file works if the config dir is writable

845cce5

Signed-off-by: Bernd Stellwag <burned@zerties.org>

Check if file modes got applied before proceeding with the test

60c1fd1

Signed-off-by: Bernd Stellwag <burned@zerties.org>

burned42 force-pushed the show_error_if_config_not_writable branch from 83086b3 to 60c1fd1 Compare October 10, 2018 17:59

MorrisJobke mentioned this pull request Nov 7, 2018

15.0.0 beta 1 #12310

Merged

29 tasks

MorrisJobke modified the milestones: Nextcloud 15, Nextcloud 16 Nov 8, 2018

ChristophWurst modified the milestones: Nextcloud 16, Nextcloud 17 Mar 1, 2019

MorrisJobke mentioned this pull request Jul 17, 2019

17.0.0 Beta 1 #16408

Merged

28 tasks

MorrisJobke modified the milestones: Nextcloud 17, Nextcloud 18 Jul 17, 2019

Merge branch 'master' into show_error_if_config_not_writable

59fac23

nickvergessen requested changes Dec 3, 2019

View reviewed changes

burned42 closed this Dec 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a more specific error message if config file is not writable #11663

Show a more specific error message if config file is not writable #11663

burned42 commented Oct 7, 2018

burned42 commented Oct 7, 2018

kesselb left a comment •

edited

Loading

kesselb Oct 7, 2018

MorrisJobke Oct 8, 2018

burned42 Oct 8, 2018 •

edited

Loading

kesselb Oct 8, 2018

burned42 Oct 8, 2018 •

edited

Loading

kesselb Oct 8, 2018

burned42 Oct 8, 2018

kesselb commented Oct 7, 2018 •

edited

Loading

burned42 commented Oct 8, 2018

burned42 commented Oct 8, 2018 •

edited

Loading

burned42 commented Oct 15, 2018

MorrisJobke commented Oct 17, 2018

MorrisJobke commented Nov 5, 2018

ChristophWurst commented Oct 24, 2019

burned42 commented Oct 24, 2019

ChristophWurst commented Oct 24, 2019

kesselb commented Oct 24, 2019 •

edited

Loading

kesselb commented Oct 25, 2019

nickvergessen Dec 3, 2019

burned42 commented Dec 3, 2019 •

edited

Loading

ChristophWurst commented Dec 18, 2019

	public static function checkConfig() {
	$l = \OC::$server->getL10N('lib');

	// Create config if it does not already exist
	$configFilePath = self::$configDir .'/config.php';
	if(!file_exists($configFilePath)) {
	@touch($configFilePath);
	}

	// Check if config is writable
	$configFileWritable = is_writable($configFilePath);
	if (!$configFileWritable && !OC_Helper::isReadOnlyConfigEnabled()
	\|\| !$configFileWritable && \OCP\Util::needUpgrade()) {

	$urlGenerator = \OC::$server->getURLGenerator();

	if (self::$CLI) {
	echo $l->t('Cannot write into "config" directory!')."\n";
	echo $l->t('This can usually be fixed by giving the webserver write access to the config directory')."\n";
	echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-dir_permissions') ])."\n";
	echo "\n";
	echo $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it.')."\n";
	echo $l->t('See %s', [ $urlGenerator->linkToDocs('admin-config') ])."\n";
	exit;
	} else {
	OC_Template::printErrorPage(
	$l->t('Cannot write into "config" directory!'),
	$l->t('This can usually be fixed by giving the webserver write access to the config directory. See %s',
	[ $urlGenerator->linkToDocs('admin-dir_permissions') ]) . '. '
	. $l->t('Or, if you prefer to keep config.php file read only, set the option "config_is_read_only" to true in it. See %s',
	[ $urlGenerator->linkToDocs('admin-config') ] ),
	503
	);
	}
	}
	}

Show a more specific error message if config file is not writable #11663

Show a more specific error message if config file is not writable #11663

Conversation

burned42 commented Oct 7, 2018

burned42 commented Oct 7, 2018

kesselb left a comment • edited Loading

Choose a reason for hiding this comment

kesselb Oct 7, 2018

Choose a reason for hiding this comment

MorrisJobke Oct 8, 2018

Choose a reason for hiding this comment

burned42 Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

kesselb Oct 8, 2018

Choose a reason for hiding this comment

burned42 Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

kesselb Oct 8, 2018

Choose a reason for hiding this comment

burned42 Oct 8, 2018

Choose a reason for hiding this comment

kesselb commented Oct 7, 2018 • edited Loading

burned42 commented Oct 8, 2018

burned42 commented Oct 8, 2018 • edited Loading

burned42 commented Oct 15, 2018

MorrisJobke commented Oct 17, 2018

MorrisJobke commented Nov 5, 2018

ChristophWurst commented Oct 24, 2019

burned42 commented Oct 24, 2019

ChristophWurst commented Oct 24, 2019

kesselb commented Oct 24, 2019 • edited Loading

kesselb commented Oct 25, 2019

nickvergessen Dec 3, 2019

Choose a reason for hiding this comment

burned42 commented Dec 3, 2019 • edited Loading

ChristophWurst commented Dec 18, 2019

kesselb left a comment •

edited

Loading

burned42 Oct 8, 2018 •

edited

Loading

burned42 Oct 8, 2018 •

edited

Loading

kesselb commented Oct 7, 2018 •

edited

Loading

burned42 commented Oct 8, 2018 •

edited

Loading

kesselb commented Oct 24, 2019 •

edited

Loading

burned42 commented Dec 3, 2019 •

edited

Loading